Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

do not allow inference in predicate_must_hold (alternative approach) #110100

Merged
merged 2 commits into from
May 19, 2023
Merged

do not allow inference in predicate_must_hold (alternative approach) #110100

merged 2 commits into from
May 19, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Apr 9, 2023 •
edited
Loading

See the FCP description for more info, but tl;dr is that we should not return EvaluatedToOkModuloRegions if an obligation may hold only with some choice of inference vars being constrained.

Attempts to solve this in the approach laid out by lcnr here: #109558 (comment), rather than by eagerly replacing infer vars with placeholders which is a bit too restrictive.

r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 9, 2023
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 9, 2023
@bors
Copy link
Contributor

bors commented Apr 9, 2023

⌛ Trying commit 0779b6620d33449d376a8b437caa060a364cd6e0 with merge dc2620d92af7fa332c95a7cd35393bf9510ac125...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-cloud-vms rust-cloud-vms bot force-pushed the no-infer-pred-must-hold branch from 0779b66 to 4230620 Compare April 9, 2023 01:34
@compiler-errors
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Apr 9, 2023

⌛ Trying commit 4230620b72959d6be0cfe37de66d63cc571c5ad2 with merge f1594433169cca61dd6137e11b86ca894968fb2f...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 9, 2023

☀️ Try build successful - checks-actions
Build commit: f1594433169cca61dd6137e11b86ca894968fb2f (f1594433169cca61dd6137e11b86ca894968fb2f)

@rust-timer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-110100 created and queued.
🤖 Automatically detected try build f1594433169cca61dd6137e11b86ca894968fb2f
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 9, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-110100 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f1594433169cca61dd6137e11b86ca894968fb2f): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.2% [0.5%, 1.6%] 7
Regressions ❌
(secondary)		 0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.2% [0.5%, 1.6%] 7

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)		 -2.0% [-2.0%, -2.0%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -2.0% [-2.0%, -2.0%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -5.6% [-11.1%, -2.9%] 4
All ❌✅ (primary) - - 0

@rustbot rustbot added the perf-regression Performance regression. label Apr 9, 2023
@craterbot
Copy link
Collaborator

🎉 Experiment pr-110100 is completed!
📊 28 regressed and 8 fixed (260976 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 10, 2023
@compiler-errors compiler-errors marked this pull request as ready for review April 13, 2023 01:17
@compiler-errors compiler-errors added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the no-infer-pred-must-hold branch from 4230620 to a15ca71 Compare April 13, 2023 19:48
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 8, 2023
@rfcbot
Copy link

rfcbot commented May 8, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors
Copy link
Contributor

bors commented May 12, 2023

☔ The latest upstream changes (presumably #109732) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-cloud-vms rust-cloud-vms bot force-pushed the no-infer-pred-must-hold branch from a15ca71 to c06e611 Compare May 12, 2023 18:54
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 18, 2023
@rfcbot
Copy link

rfcbot commented May 18, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label May 18, 2023
@jackh726
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented May 19, 2023

📌 Commit c06e611 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2023
@bors
Copy link
Contributor

bors commented May 19, 2023

⌛ Testing commit c06e611 with merge 19ca569...

@bors
Copy link
Contributor

bors commented May 19, 2023

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 19ca569 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2023
@bors bors merged commit 19ca569 into rust-lang:master May 19, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 19, 2023
@bors bors mentioned this pull request May 19, 2023
Merged
2 tasks
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (19ca569): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 1.1% [0.9%, 1.4%] 6
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) 1.1% [0.9%, 1.4%] 6

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 5.8% [5.8%, 5.8%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 641.663s -> 641.09s (-0.09%)

@lqd
Copy link
Member

lqd commented May 19, 2023

Correct me if I'm wrong, but the perf hit looks expected, and since this PR looks like this is a correctness fix, it's deemed acceptable.

@pnkfelix
Copy link
Member

pnkfelix commented May 23, 2023 •
edited
Loading

Hey @compiler-errors and @lcnr , based on https://github.com/rust-lang/rust/pull/110100/files#diff-cacebb74c41e0865cc830e29d5404a2c69a9901c37e4f1b579eb383ed71b12c8 going from run-pass to compile-fail, this seems like it could be a breaking change, ...

... but I don't see any crater run showing me that I need not worry ...

... nor a link to an existing issue describing what is being changed here (i.e. what issues are being fixed by this change)...

... and the description here refers to some T-types FCP but I have no idea where that is...

(Update: The things I was asking for were on this PR, just hidden under the fold.)

@pnkfelix
Copy link
Member

Correct me if I'm wrong, but the perf hit looks expected, and since this PR looks like this is a correctness fix, it's deemed acceptable.

I am going to assume from context (namely the 👍 from @lcnr ) that this is indeed a correctness fix of some form, and therefore the perf regression is expected and acceptable.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label May 23, 2023
@compiler-errors
Copy link
Member Author

@pnkfelix look in the comments above for crater, FCP, and description of changes.

@pnkfelix
Copy link
Member

sorry for the noise!

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
@compiler-errors compiler-errors deleted the no-infer-pred-must-hold branch August 11, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jackh726 jackh726

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.